-
Notifications
You must be signed in to change notification settings - Fork 283
Remove AccessToken::is_expired() #2611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the misleading AccessToken::is_expired() method and replaces it with a more robust token refresh mechanism while also adding test coverage and fixing an error in the proactive token refresh logic.
- Removed AccessToken::is_expired() and updated the token checking logic in cache and bearer token policy.
- Implemented a new helper (should_refresh) to determine token refresh timing.
- Enhanced error handling during token refresh and updated tests accordingly.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
sdk/identity/azure_identity/src/credentials/cache.rs | Replaced usage of is_expired() with should_refresh and updated test expiration. |
sdk/core/azure_core/src/http/policies/bearer_token_policy.rs | Refactored token refresh logic and improved error handling during refresh. |
sdk/core/azure_core/src/credentials.rs | Removed the is_expired() method as part of the deprecation. |
sdk/core/azure_core/CHANGELOG.md | Documented the breaking change and bug fix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I see nothing blocking. Just a couple of notes and one suggestion.
...because it's unnecessary and its name is misleading. This PR also adds test coverage and fixes a bug: the policy currently returns errors from proactive refresh attempts even when it has a valid cached token. (There's no value in doing that because the client request could otherwise succeed.)